Issues backlog: PhoneApp onDestroyed, custom commands enhancement, employee appearances, more checks in CombatBehaviour#84
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR enhances console command registration, adds a type-safe employee appearance API, and improves error handling with centralized logging and component lifecycle management. The custom console registry is exposed to allow UI injection patches to register commands in the in-game command list, the new EmployeeManager provides safe access to base-game employee data, and CombatBehaviour refactors error reporting through Logger while PhoneAppButtonHandler ensures proper cleanup. ChangesCustom Console Commands UI Integration
Employee Appearance Wrapper API
Error Handling and Component Lifecycle Management
Sequence Diagram(s)sequenceDiagram
participant CommandListScreen
participant ConsolePatches
participant CustomConsoleRegistry
participant UIContainer
CommandListScreen->>ConsolePatches: S1CommandListScreen.Start (Harmony postfix)
ConsolePatches->>CustomConsoleRegistry: iterate registry.Keys
loop For each custom command
ConsolePatches->>UIContainer: instantiate command entry prefab
ConsolePatches->>UIContainer: populate Command/Description/Example text
ConsolePatches->>UIContainer: append entry to commandEntries
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
S1API/Console/CustomConsoleRegistry.cs (1)
14-14: ⚡ Quick winKeep the registry encapsulated.
ConsolePatchesonly needs to enumerate commands. Making the field itselfinternallets any internal caller bypassRegister()'s trim/empty-key checks and mutate the dictionary directly. Expose a read-only view or iterator instead.Suggested change
- internal static readonly Dictionary<string, BaseConsoleCommand> registry = new Dictionary<string, BaseConsoleCommand>(StringComparer.OrdinalIgnoreCase); + private static readonly Dictionary<string, BaseConsoleCommand> registry = new Dictionary<string, BaseConsoleCommand>(StringComparer.OrdinalIgnoreCase); + internal static IReadOnlyDictionary<string, BaseConsoleCommand> Registry => registry;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Console/CustomConsoleRegistry.cs` at line 14, The registry field is currently internal which allows bypassing Register() validations; make the dictionary private (private static readonly Dictionary<string, BaseConsoleCommand> registry) and expose a read-only view or enumerator instead (for example a public/static IReadOnlyDictionary<string, BaseConsoleCommand> or a method IEnumerable<KeyValuePair<string,BaseConsoleCommand>> GetRegisteredCommands()) so ConsolePatches can enumerate without mutating; keep Register() (and its trim/empty-key checks) as the single mutating entry point and return or expose only the read-only wrapper from the new accessor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@S1API/Entities/Behaviour/CombatBehaviour.cs`:
- Around line 92-93: The DefaultWeaponAssetPath setter currently calls
Object.Instantiate(gameObject) and then GetComponent<AvatarEquippable>, which
creates an undeleted runtime clone; instead, remove the Instantiate call and
validate the assigned prefab directly by calling
GetComponent<AvatarEquippable>() or GetComponentInChildren<AvatarEquippable>()
on the prefab reference (or this.gameObject/prefab variable used in the setter)
to check for AvatarEquippable, and keep the existing null-handling (log/throw)
path if the component is missing.
In `@S1API/Entities/Employees/EmployeeManager.cs`:
- Line 33: The wrapper must guard against nulls from the underlying API: change
the EmployeeManager appearance wrapper so that when
S1Employees.EmployeeManager.Instance.GetAppearance(male, index) returns null you
return null (or propagate appropriately) instead of always calling new
EmployeeAppearance(...); similarly update GetRandomAppearance to use the out
AvatarSettings? avatarSettings only if the base call returns true and
avatarSettings is non-null — do not always construct new
AvatarSettings(avatarSettings) or always return true; instead call
S1Employees.EmployeeManager.Instance.GetRandomAppearance(...) and if it returns
false or the out avatarSettings is null, set the wrapper out to null and return
false, otherwise construct new AvatarSettings(avatarSettings) and return true.
Ensure you reference EmployeeAppearance, EmployeeManager.Instance.GetAppearance,
GetRandomAppearance, AvatarSettings and the avatarSettings out parameter when
applying the checks.
In `@S1API/Internal/Patches/ConsolePatches.cs`:
- Line 43: The static HashSet _addedCommandsToList is being cleared only in
AddCommands() via S1Console.Awake which prevents newly created
S1CommandListScreen instances from receiving custom entries; move or add the
reset so duplicate tracking is cleared at the start of each command-list screen
creation by clearing _addedCommandsToList at the beginning of
AddCustomCommandEntries() (or change dedupe to an instance-scoped set inside
S1CommandListScreen) so each S1CommandListScreen gets its own fresh tracking and
custom keys are added correctly.
- Around line 174-189: The postfix currently adds custom command entries even
when their CommandWord collides with a built-in command; before instantiating or
adding an entry inside the foreach over CustomConsoleRegistry.registry.Keys,
check for collisions with the game's native registry (e.g., S1Console.commands
or the equivalent built-in command map) and skip entries whose key already
exists there so only executable custom commands are shown; update the logic
around CustomConsoleRegistry.registry, _addedCommandsToList, CommandEntryPrefab
and CommandEntryContainer to continue when a built-in command owns the key.
In `@S1API/PhoneApp/PhoneApp.cs`:
- Around line 651-655: OnDestroy currently unconditionally calls DestroyInternal
and can re-enter destruction when PhoneApp.OnDestroyed destroys _appPanel (which
tears down the handler); add a re-entrancy guard: introduce a private bool
(e.g., _isDestroying) on PhoneApp, set it true at the start of DestroyInternal
(or OnDestroyed) and skip any further destruction if it's already true, and
update OnDestroy to check that flag (or phoneApp != null &&
!phoneApp._isDestroying) before calling DestroyInternal to prevent double runs.
---
Nitpick comments:
In `@S1API/Console/CustomConsoleRegistry.cs`:
- Line 14: The registry field is currently internal which allows bypassing
Register() validations; make the dictionary private (private static readonly
Dictionary<string, BaseConsoleCommand> registry) and expose a read-only view or
enumerator instead (for example a public/static IReadOnlyDictionary<string,
BaseConsoleCommand> or a method
IEnumerable<KeyValuePair<string,BaseConsoleCommand>> GetRegisteredCommands()) so
ConsolePatches can enumerate without mutating; keep Register() (and its
trim/empty-key checks) as the single mutating entry point and return or expose
only the read-only wrapper from the new accessor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddf3d5e3-e041-4fb5-ab8d-cc937bd56ae3
📒 Files selected for processing (5)
S1API/Console/CustomConsoleRegistry.csS1API/Entities/Behaviour/CombatBehaviour.csS1API/Entities/Employees/EmployeeManager.csS1API/Internal/Patches/ConsolePatches.csS1API/PhoneApp/PhoneApp.cs
Closes a bunch of issues: #74, #76, #83.
Also closes one issue from Trello - custom commands now appear in the command list screen, just like native commands do.
Release Notes
OnDestroy()wiring forPhoneAppButtonHandlerto properly tear down the associatedPhoneAppwhen the button handler component is destroyedEmployeeManagerandEmployeeAppearancewrapper classes with access to appearance settings and mugshotsCombatBehaviour.DefaultWeaponAssetPathsetter with safer type casting usingCrossType.Ischecks, switched to melonlogger for error reporting, and updated XML documentationCustomConsoleRegistry.registryinternal to support assembly-level access for custom command integrationContributions by Author